My initial reaction to this is that it seems complex for what it is.

Why couldn't we just impose on the NetworkTransport to use NGO's enum when setting a disconnect reason? That is, only add SetDisconnectEvent(DisconnectEvents disconnectEvent, string message = null) to the NetworkTransport API. Every transport can then do its own mapping internally in the way it makes the most sense for it (possibly in a more efficient way than by maintaining a dictionary on the side just for that).

I'm not sure I see the point of having all that mapping machinery in NetworkTransport. Plus, it kinda bakes the idea that a transport's disconnect reason will be a byte (or even just a numerical value) directly in the API. That's true for UnityTransport, but might not be the case for other transports. Say if a transport has their backend returning strings explaining the disconnect reason, then all of that mapping machinery is basically useless for them. They'll end up doing their own mapping of strings to disconnect events directly in their transport.

My initial reaction to this is that it seems complex for what it is.

Why couldn't we just impose on the NetworkTransport to use NGO's enum when setting a disconnect reason? That is, only add SetDisconnectEvent(DisconnectEvents disconnectEvent, string message = null) to the NetworkTransport API. Every transport can then do its own mapping internally in the way it makes the most sense for it (possibly in a more efficient way than by maintaining a dictionary on the side just for that).

I'm not sure I see the point of having all that mapping machinery in NetworkTransport. Plus, it kinda bakes the idea that a transport's disconnect reason will be a byte (or even just a numerical value) directly in the API. That's true for UnityTransport, but might not be the case for other transports. Say if a transport has their backend returning strings explaining the disconnect reason, then all of that mapping machinery is basically useless for them. They'll end up doing their own mapping of strings to disconnect events directly in their transport.

The approach you describe above I actually started with, but then I realized that I would have to decorate all places that implement the feature/update with UTP_TRANSPORT_2_4_ABOVE. Which would mean any custom NetworkTransport would have no way of having messages handled without including com.unity.transport. The higher abstraction ensures that any NetworkTransport derived class can provide a map to the "universal disconnect events" based on the values the transport in question provides.

Otherwise, users would always have to:

• Include com.unity.transport
• Replicate a chunk of the "mapping" code in this PR to map the transport they are adding to the UTP disconnect events.

This way, it keeps the notification "transport neutral" while providing some mechanism to register a transport's disconnect events to the NGO (standardized) disconnect events.

Does this make more sense?

Does this make more sense?

Not really. Maybe I'm missing something obvious, but I don't see how UTP would be required.

To be clear, my suggestion is not for the disconnection events in NetworkTransport to be UTP's. I'm proposing that NGO has its own enum for disconnection events (like it does in this PR), but that the only API in NetworkTransport be to signal those NGO disconnection events, and not provide any mapping machinery. If there's a need for a mapping between those NGO events and the transport's, then that mapping should be in the transport itself.

Basically my proposal would be:

abstract class NetworkTransport
{
    public enum DisconnectEvents
    {
        ...
    }

    public DisconnectEvents DisconnectEvent { get; protected set; }
    public string DisconnectEventMessage { get; protected set; }
}

public class UnityTransport
{
    ...
    case TransportEvent.Disconnect:
        ...
        switch (reader.ReadByte())
        {
            case (byte)Error.Timeout:
                DisconnectEvent = DisconnectEvents.ProtocolTimeout;
                DisconnectEventMessage = "Connection has timed out.";
            ...
        }
}

And while I get the argument that we don't want every transport to have to implement mapping logic, I don't agree with it. The mapping logic will normally be simple enough that I don't believe it's worth abstracting away in NetworkTransport. In the case of UnityTransport, it can be as simple as a switch statement. And it's not like the switch statement is more verbose or anything; the current proposal has basically the same amount of code required to set up the mappings through the generic mechanism.

the same amount of code required to set up the mappings through the generic mechanism.

Yeah...you have a good point about the amount of code and I wasn't 100% happy with my initial approach anyway.... I will tinker around with it and see if we can keep some of the non-event type related script while approaching it the way you described above.

Regarding the new design:

• I'm not sure I understand the point of OnGetDisconnectEventMessage. If a transport is interested in providing a custom message to go with their error, couldn't they just pass it to SetDisconnectEvent? Plus it's a bit weird to have a OnGetDisconnectEventMessage function when there is no GetDisconnectEventMessage.
• What is the purpose of UnityTransportNotificationHandler and why does it need to be a separate class that handles dynamic mappings through dictionaries? Maybe I'm missing something obvious, but couldn't it be just one big switch case at the point where we want to set the disconnection event (when we read the disconnection reason byte)?

@simon-lemay-unity

Regarding the new design:

• I'm not sure I understand the point of OnGetDisconnectEventMessage. If a transport is interested in providing a custom message to go with their error, couldn't they just pass it to SetDisconnectEvent? Plus it's a bit weird to have a OnGetDisconnectEventMessage function when there is no GetDisconnectEventMessage.

So, if you look at the ClosingRemoteConnection and ShuttingDown methods within NetworkTransport, those are two NGO transport layer event notifications that are not generated by UnityTransport. To make this relatively user friendly, I have opted to go with providing more than one way to handle disconnect notifications/messages:

• The ones that the actual transport provides.
  • Within UnityTransport.ProcessEvent during the TransportEvent.Disconnect event.
    • To provide a disconnection event notification, it just has to translate the UTP disconnect event to the NGO transport layer disconnect event.
• The same kind of pattern can be used in other custom transports.

There are two options a user has:

• Set the disconnect event only and allow the OnGetDisconnectEventMessage (part of the answer to your question) handle getting the message.
• Set the disconnect event and message at the same time.

So... I could optionally make this adjustment in UnityTransportNotificationHandler:

        internal (NetworkTransport.DisconnectEvents, string) GetDisconnectEvent(int disconnectEventId)
        {
            var disconnectEvent = m_DisconnectEventMap.ContainsKey(disconnectEventId)? m_DisconnectEventMap[disconnectEventId] : NetworkTransport.DisconnectEvents.Disconnected;
            var disconnectMessage = m_DisconnectEventMessageMap.ContainsKey(disconnectEvent) ? m_DisconnectEventMessageMap[disconnectEvent] : string.Empty;
            return (disconnectEvent, disconnectMessage);
        }

Where within UnityTransport.ProcessEvent during the TransportEvent.Disconnect event it could look like this:

case TransportEvent.Disconnect:
    {
        // If we're a client and had not yet set the server client ID, it means
        // our connection to the server failed to be established. Any other case
        // means a clean disconnect that doesn't require logging.
        if (!m_Driver.Listening && m_ServerClientId == default)
        {
            Debug.LogError("Failed to connect to server.");
        }

        // Set the disconnect event error code
        var errorCode = m_UnityTransportNotificationHandler.GetDisconnectEvent(reader.ReadByte());
        SetDisconnectEvent(errorCode.Item1, errorCode.Item2);

This approach will handle all DisconnectEvents that can be mapped to the transport in question. However, there are 2 remaining disconnect event notifications that UTP does not return but are still valid and should have a way to map a message to them:

DisconnectEvents.ClosedRemoteConnection: This is invoked on the instance that closes the connection to provide additional information (i.e. the local application forced the disconnect).

DisconnectEvents.TransportShutdown: This is invoked upon the transport being shutdown. For third party transports like Steam this can be useful to know and provide additional reasons as to why the transport was shutdown.

To answer the question as to why OnGetDisconnectEventMessage (I can change that to just GetDisconnectMessage), it provides a way to handle any DisconnectEvents value that the transport does not provide but the author of the NGO transport wants to adjust the message for .

When I was putting this together for UnityTransport, I could have opted to go with the above alternate way of handling the assignment of the message... but would still have to override OnGetDisconnectEventMessage in order to handle the two events that are not a "transport specific disconnect event"... so it was "valid" to just set the DisconnectEvents value when UnityTransport was processing a disconnect event and let the overridden OnGetDisconnectEventMessage return the message since it was needed anyway to provide a transport specific message for ClosedRemoteConnection and TransportShutdown.

• What is the purpose of UnityTransportNotificationHandler and why does it need to be a separate class that handles dynamic mappings through dictionaries? Maybe I'm missing something obvious, but couldn't it be just one big switch case at the point where we want to set the disconnection event (when we read the disconnection reason byte)?

I could completely move all of UnityTransportNotificationHandler into UnityTransport, but it seemed cleaner and it seemed like the pattern already used was to create a separate class to handle other things... like ErrorUtilities which could just as easily be added to UnityTransport as well. It just seemed cleaner to keep this separate and users could literally copy and paste the class, make adjustments and add it to their own custom transport.

The reason behind not just making a large switch statement that had everything in one place I guess is a matter of coding style/preference. At some point, we could need to provide a way for users to localize messages for different languages and so I figured we should start designing anything that returns some form of message that could be localized in a way that is easy to find, understand, and modify without impacting any of the logic within UnitTransport itself (or any other customer transport).

The reason behind the OnGetDisconnectEventMessage (will probably rename this as you make a good point about the name) is to provide users a way to add unique messages for a custom transport that does not provide those disconnect event types.